Multiple database support #31
Conversation
WalkthroughThis update introduces multi-database support to the connector by replacing single PostgreSQL client instances with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Connector
participant ClientDatabasesPool
participant Client
Caller->>Connector: Request resource sync (e.g., list tables)
Connector->>ClientDatabasesPool: Parse resource ID for db, get client (Get)
ClientDatabasesPool->>Client: Return client for db (create if needed)
Connector->>Client: Perform operation (e.g., ListTables)
Client-->>Connector: Return results
Connector-->>Caller: Return resources
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (2)pkg/connector/view.go (1)
pkg/connector/sequence.go (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
pkg/connector/table.go (1)
83-108:⚠️ Potential issuePossible misuse of the default client for cross‑database lookup
r.clientPool.Default(ctx)is used to fetch a database record by ID.
If the DSN contained a database name, the “default” client will be pinned to that DB andpg_databaselook‑ups for other DBs could fail (or at best be inconsistent).Consider fetching against any healthy client instead, or even the same client already used for the DB in question:
- dbModel, err := r.clientPool. - Default(ctx). - GetDatabaseById(ctx, dbId) + dbClient, _, err := r.clientPool.Get(ctx, dbId) + if err != nil { + return nil, "", nil, err + } + dbModel, err := dbClient.GetDatabaseById(ctx, dbId)This guarantees the query runs in the correct database context.
♻️ Duplicate comments (3)
pkg/connector/view.go (1)
98-109: Same nil‑pointer hazard inGrants– see earlier comment; the fix is identical.pkg/connector/sequence.go (1)
98-109: Repeats thepTokennil‑check issue – see first comment in this file.pkg/connector/table.go (1)
118-133: Nil‑pointer concern forpTokenagain – identical fix as above.
🧹 Nitpick comments (8)
pkg/postgres/databases.go (1)
80-98: Consider type conversion for database ID.The new method accepts a database ID as a string but uses it directly in the query against the OID column (which is an integer type). While PostgreSQL may handle the type conversion implicitly, it would be safer to parse the string to an integer first to avoid potential type mismatch issues.
Also, consider maintaining naming consistency with existing methods (e.g.,
dbIdvsdbIDused elsewhere).-func (c *Client) GetDatabaseById(ctx context.Context, dbId string) (*DatabaseModel, error) { +func (c *Client) GetDatabaseById(ctx context.Context, dbID string) (*DatabaseModel, error) { ret := &DatabaseModel{} q := ` SELECT "oid"::int, "datname", "datdba", "datacl" from "pg_catalog"."pg_database" WHERE "oid"=$1 ` - err := pgxscan.Get(ctx, c.db, ret, q, dbId) + oid, err := strconv.ParseInt(dbID, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid database ID: %w", err) + } + + err = pgxscan.Get(ctx, c.db, ret, q, oid) if err != nil { return nil, err } return ret, nil }pkg/connector/connector.go (1)
64-67: Error handling could be more specific.While the changes are correct, consider enhancing the error message to provide more context about potential failure scenarios specific to the client pool creation.
- return nil, fmt.Errorf("failed to create postgres client pool: %w", err) + return nil, fmt.Errorf("failed to create postgres client pool (check DSN and connection permissions): %w", err)pkg/connector/function.go (1)
92-94: Minor: keep entitlement display names stableRe‑prefixing every entitlement with the database name changes display strings that consumers may already persist for audit logs. If that’s intentional – great. If the intention is disambiguation only when the same entitlement slug exists in multiple DBs, consider keeping the original
DisplayNameand instead add the DB context toDescriptionor an annotation field.pkg/connector/database.go (1)
5-10: Rungoimportsto normalise import groupingStatic analysis flagged the file as not
goimports‑formatted. Running:goimports -w pkg/connector/database.gowill group std‑lib, third‑party and local imports, avoiding CI failures in strict pipelines.
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 6-6:
File is not properly formatted (goimports)pkg/connector/schema.go (1)
51-54: Remove superfluous blank line to satisfygo vet/golint.There’s an extra empty line after the early‑return that triggers the “unnecessary trailing newline” lint warning.
- if dbName == "" { - return nil, "", nil, fmt.Errorf("database name not found for ID %d", dbId) - - } + if dbName == "" { + return nil, "", nil, fmt.Errorf("database name not found for ID %d", dbId) + }🧰 Tools
🪛 GitHub Check: go-lint
[failure] 54-54:
unnecessary trailing newline (whitespace)pkg/postgres/client.go (1)
3-10: Rungoimportsto sort imports and remove unused ones.The file is currently flagged by
goimports/golint. Running the tool will:
- Group standard‑library imports first.
- Remove
errorsif no longer used after other fixes, or ensure it’s grouped correctly.Keeping imports tidy prevents CI failures and improves readability.
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 5-5:
File is not properly formatted (goimports)pkg/connector/view.go (1)
73-81:TODOleft unaddressed for entitlementsThe comment
// TODO: add datindicates incomplete entitlement handling. This may lead to missing privileges in downstream access‑reviews.
Would you like help fleshing this out?pkg/connector/sequence.go (1)
79-81: Missing sequence‑level entitlements
// TODO: add entitlements for sequencesshould be addressed before shipping; otherwise users cannot request access to sequences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (171)
README.mdis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/CHANGELOG.mdis excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_CreateEventSourceMapping.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_DeleteEventSourceMapping.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_GetEventSourceMapping.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/api_op_UpdateEventSourceMapping.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/go_module_metadata.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/types/enums.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/types/errors.gois excluded by!vendor/**and included by nonevendor/github.com/aws/aws-sdk-go-v2/service/lambda/types/types.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_baton_id.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_baton_id.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/config.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/config.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/annotations/annotations.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/auth/jwt.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/bid/bid.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/bid/parser.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/bid/scanner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/commands.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/lambda_server__added.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/config/config.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorbuilder/connectorbuilder.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/defaults.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/lambda/grpc/config/config.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/lambda/grpc/middleware/auth.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/lambda/grpc/server.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/provisioner/provisioner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sdk/version.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/syncer.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/resource/user_trait.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uotel/config.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uotel/uotel.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/dpop/integrations/dpop_oauth2/token_client_assertion.gois excluded by!vendor/**and included by nonevendor/github.com/go-jose/go-jose/v4/CONTRIBUTING.mdis excluded by!vendor/**and included by nonevendor/github.com/go-jose/go-jose/v4/README.mdis excluded by!vendor/**and included by nonevendor/github.com/go-jose/go-jose/v4/jwe.gois excluded by!vendor/**and included by nonevendor/github.com/go-jose/go-jose/v4/jwk.gois excluded by!vendor/**and included by nonevendor/github.com/go-jose/go-jose/v4/jws.gois excluded by!vendor/**and included by nonevendor/github.com/grpc-ecosystem/grpc-gateway/v2/runtime/query.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/.travis.ymlis excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/README.mdis excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/api.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/cacheobject/directive.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/cacheobject/lex.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/cacheobject/object.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/cacheobject/reasons.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/cacheobject/warning.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/cachecontrol/doc.gois excluded by!vendor/**and included by nonevendor/github.com/pquerna/xjwt/xkeyset/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/pquerna/xjwt/xkeyset/keyset.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/LICENSEis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/convert.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/core.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/encoder.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/contrib/bridges/otelzap/gen.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/.gitignoreis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/.golangci.ymlis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/CHANGELOG.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/CONTRIBUTING.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/Makefileis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/RELEASING.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/dependencies.Dockerfileis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/LICENSEis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/client.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/config.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/doc.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/exporter.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/retry/retry.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/internal/transform/log.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc/version.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/DESIGN.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/LICENSEis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/doc.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/embedded/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/embedded/embedded.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/global/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/global/log.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/internal/global/log.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/internal/global/state.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/keyvalue.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/kind_string.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/logger.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/noop/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/noop/noop.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/provider.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/record.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/severity.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/log/severity_string.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/renovate.jsonis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/requirements.txtis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/DESIGN.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/LICENSEis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/batch.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/doc.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/exporter.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/filter_processor.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/logger.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/processor.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/provider.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/record.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/ring.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/setting.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/log/simple.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/resource/os_release_darwin.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/resource/resource.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/trace/batch_span_processor.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/trace/sampling.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/trace/simple_span_processor.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/sdk/version.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/README.mdis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/attribute_group.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/doc.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/exception.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/metric.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/semconv/v1.27.0/schema.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/auto.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/attr.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/doc.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/id.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/number.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/resource.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/scope.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/span.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/status.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/traces.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/internal/telemetry/value.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/trace/noop.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/version.gois excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/otel/versions.yamlis excluded by!vendor/**and included by nonevendor/go.opentelemetry.io/proto/otlp/collector/logs/v1/logs_service.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/go.opentelemetry.io/proto/otlp/collector/logs/v1/logs_service.pb.gw.gois excluded by!**/*.pb.gw.go,!vendor/**and included by nonevendor/go.opentelemetry.io/proto/otlp/collector/logs/v1/logs_service_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/go.opentelemetry.io/proto/otlp/logs/v1/logs.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/golang.org/x/net/context/ctxhttp/ctxhttp.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer/balancer.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer/endpointsharding/endpointsharding.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer/pickfirst/pickfirstleaf/pickfirstleaf.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer/roundrobin/roundrobin.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer/subconn.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/balancer_wrapper.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/binarylog/grpc_binarylog_v1/binarylog.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/google.golang.org/grpc/clientconn.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/dialoptions.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/health/grpc_health_v1/health.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/google.golang.org/grpc/internal/balancer/gracefulswitch/gracefulswitch.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/envconfig/xds.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/grpcsync/oncefunc.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/internal.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/proxyattributes/proxyattributes.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/resolver/delegatingresolver/delegatingresolver.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/transport/http2_client.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/transport/proxy.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/internal/transport/transport.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/picker_wrapper.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/resolver/resolver.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/resolver_wrapper.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/rpc_util.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/server.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/stream.gois excluded by!vendor/**and included by nonevendor/google.golang.org/grpc/version.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (14)
go.mod(6 hunks)pkg/connector/column.go(4 hunks)pkg/connector/connector.go(2 hunks)pkg/connector/database.go(4 hunks)pkg/connector/function.go(4 hunks)pkg/connector/helpers.go(3 hunks)pkg/connector/procedure.go(5 hunks)pkg/connector/schema.go(5 hunks)pkg/connector/sequence.go(5 hunks)pkg/connector/table.go(5 hunks)pkg/connector/view.go(5 hunks)pkg/postgres/client.go(1 hunks)pkg/postgres/databases.go(1 hunks)pkg/postgres/tables.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
pkg/postgres/databases.go (1)
pkg/postgres/client.go (1)
Client(130-134)
pkg/postgres/tables.go (1)
pkg/postgres/client.go (1)
Client(130-134)
pkg/connector/function.go (2)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)pkg/postgres/privileges.go (1)
Execute(106-106)
pkg/connector/column.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/database.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/sequence.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/table.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/procedure.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/view.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
pkg/connector/schema.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(12-21)
🪛 GitHub Check: go-lint
pkg/connector/helpers.go
[failure] 18-18:
Comment should end in a period (godot)
pkg/connector/database.go
[failure] 6-6:
File is not properly formatted (goimports)
pkg/postgres/client.go
[failure] 5-5:
File is not properly formatted (goimports)
pkg/connector/schema.go
[failure] 54-54:
unnecessary trailing newline (whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (10)
pkg/postgres/tables.go (3)
51-51: Method signature updated to support multi-database functionality.The change from accepting a schema ID (int64) to a schema name (string) aligns with the PR objective of supporting multiple databases. This modification allows for more flexible schema identification across different databases.
65-65: SQL query correctly modified to filter by schema name.The WHERE clause has been appropriately updated to filter tables by schema name (
nspname) rather than by schema ID (OID), which is consistent with the method signature change.
69-69: Query parameter correctly updated to use schema name.The argument passed to the query has been properly updated to use the schema name parameter.
go.mod (2)
6-6: Dependencies updated to support multi-database functionality.The updates to baton-sdk (v0.2.93) and PostgreSQL-related dependencies are appropriate for supporting the multi-database functionality introduced in this PR.
Also applies to: 10-10
101-109: New OpenTelemetry dependencies added.Several OpenTelemetry dependencies have been added, which should enhance the observability of the application. This is a good practice when introducing new functionality like multi-database support.
pkg/connector/connector.go (3)
15-15: Client field updated to use ClientDatabasesPool.The change from a single client to a client pool correctly implements the multi-database support objective of this PR.
23-32: ResourceSyncers implementation correctly uses client pool.The ResourceSyncers method now appropriately uses either the default client from the pool or passes the entire pool to syncers as needed. This allows syncers to obtain the correct client for each database operation.
70-70: Struct initialization correctly updated.The struct initialization properly uses the new clientPool field name.
pkg/connector/column.go (1)
40-48: Same edge‑case as function syncer: ensure DB component existsIf
parseWithDatabaseIDreturns an empty database string we end up defaulting to the implicit client, defeating multi‑DB isolation. Propagate a clear error whendb == ""so callers understand the resource ID is malformed or from an earlier connector version.pkg/postgres/client.go (1)
69-81: Clarify thedatabaseparameter semantics.
Getexpects the database OID (as string), not the database name, but callers may confuse the two. This is implicit and easy to misuse.Recommendation:
- Rename the parameter to
databaseID string.- Add a doc comment explicitly stating it must be the numeric OID.
- Consider overloading with
GetByNameif you foresee name‑based look‑ups.Clearer naming will prevent accidental bugs when other contributors extend this pool.
| @@ -1,14 +1,13 @@ | |||
| module github.com/conductorone/baton-postgresql | |||
|
|
|||
| go 1.23.4 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Go version specification.
The Go version is specified as 1.23.4, which appears to be ahead of the latest released version as of when this review is being conducted. Consider verifying if this is the correct version you intend to use or if it should be updated to the latest stable release.
🌐 Web query:
What is the latest released version of Go programming language?
💡 Result:
The latest released version of the Go programming language is Go 1.24.2, which was released on April 1, 2025[1][3][8]. This release includes security fixes, as well as bug fixes for the compiler, runtime, the go command, and various standard library packages such as crypto/tls, go/types, net/http, and testing[1].
Here’s a summary of recent Go releases:
| Version | Release Date | Notable Notes |
|---|---|---|
| 1.24.2 | 2025-04-01 | Latest stable release (security and bug fixes) |
| 1.24.1 | 2025-03-04 | Security and bug fixes |
| 1.24.0 | 2025-02-11 | Major release with new features |
Go 1.25 is not yet released as of mid-April 2025[7].
For the latest updates and details on new features, you can consult the official Go release history[1][3][8].
Citations:
- 1: https://go.dev/doc/devel/release
- 2: https://www.geeksforgeeks.org/future-of-golang/
- 3: https://tip.golang.org/doc/go1.24
- 4: https://dev.to/empiree/why-you-should-learn-go-in-2025-308l/comments
- 5: https://ssojet.com/blog/unlocking-the-future-of-golang-trends-predictions-and-business-impact-in-2025/
- 6: https://github.com/golang/tools/releases
- 7: https://tip.golang.org/doc/next
- 8: https://en.wikipedia.org/wiki/Go_(programming_language)
- 9: https://bitfieldconsulting.com/posts/rust-vs-go
- 10: https://tip.golang.org/doc/go1.23
Update Go version to the latest stable release
The go directive in go.mod is currently set to 1.23.4, which is behind the latest stable release (Go 1.24.2 as of April 1, 2025). Please update it to benefit from the most recent security patches and bug fixes.
• File: go.mod
Line 3:
-go 1.23.4
+go 1.24.2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go 1.23.4 | |
| go 1.24.2 |
| db, parentID, err := parseWithDatabaseID(parentResourceID.Resource) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| functions, nextPageToken, err := r.client.ListFunctions(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| client, _, err := r.clientPool.Get(ctx, db) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against resources that don’t embed a database ID
parseWithDatabaseID assumes the incoming resource string always contains a database component. If a caller hands us a schema ID that was produced by an older Baton‑PostgreSQL build (i.e. pre‑multi‑DB), db will be empty and the subsequent clientPool.Get will return the default client, silently re‑introducing single‑DB semantics. That will surface later as confusing “object not found” errors.
Consider short‑circuiting the call when db == "", logging a warning and returning a typed error so the UI can reflect the unsupported ID format.
There was a problem hiding this comment.
We don't need the db name in this scenario
There was a problem hiding this comment.
@MarcusGoldschmidt I understand. Thanks for clarifying that the database name isn't needed in this specific scenario. This suggests that either your design already accounts for the case where a database ID might be empty, or that the fallback to the default client is the expected behavior here.
| dbModel, err := r.clientPool. | ||
| Default(ctx). | ||
| GetDatabaseById(ctx, dbId) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Use a database‑scoped client instead of the default for metadata look‑ups
GetDatabaseById is executed on clientPool.Default(ctx).
When the DSN contains no database, the default client connects to the server without selecting a DB, which means:
search_pathmay not include the requested DB, producing “undefined table” style errors.- RLS / pg_hba rules might block the query even though connecting directly to that DB would succeed.
Retrieving the database model through the same client that will later be used for entitlement checks avoids those edge cases:
- dbModel, err := r.clientPool.
- Default(ctx).
- GetDatabaseById(ctx, dbId)
+ scopedClient, _, err := r.clientPool.Get(ctx, dbId)
+ if err != nil {
+ return nil, "", nil, err
+ }
+ dbModel, err := scopedClient.GetDatabaseById(ctx, dbId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbModel, err := r.clientPool. | |
| Default(ctx). | |
| GetDatabaseById(ctx, dbId) | |
| if err != nil { | |
| scopedClient, _, err := r.clientPool.Get(ctx, dbId) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| dbModel, err := scopedClient.GetDatabaseById(ctx, dbId) | |
| if err != nil { |
There was a problem hiding this comment.
We check if the database is connectable, if not we don't sync child resources
There was a problem hiding this comment.
@MarcusGoldschmidt You're right that you check database connectivity in the databaseSyncer.List method and skip databases that aren't connectable.
However, CodeRabbit's suggestion was actually about a different issue: when accessing metadata for a database that is connectable, using the default client might still cause problems with search_path or permission settings. The database-scoped client (from Get(ctx, dbId)) would have the correct context, while the default client might not.
That said, if you have specific reasons for using the default client in this case or have verified that it works correctly in your environment, feel free to keep the current implementation.
✏️ Learnings added
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-postgresql#31
File: pkg/connector/function.go:79-0
Timestamp: 2025-04-17T20:26:27.564Z
Learning: In the baton-postgresql connector, database connectivity is checked in the databaseSyncer.List method. Databases that aren't connectable (error code 55000) are logged and skipped from being synced.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| _, _, err := r.clientPool.Get(ctx, fmt.Sprintf("%d", o.ID)) | ||
| if err != nil { | ||
| var pgErr *pgconn.PgError | ||
| if errors.As(err, &pgErr) { | ||
| // Database does not accept connections | ||
| if pgErr.Code == "55000" { | ||
| l.Info("skipping database that does not accept connections", zap.String("database", o.Name), zap.Error(err)) | ||
| continue | ||
| } | ||
| } | ||
| return nil, "", nil, err | ||
| } |
There was a problem hiding this comment.
Connect‑test the database by name, not by numeric OID
ClientDatabasesPool.Get keys the cache by database name. Passing the numeric OID creates a second, uncached entry, so every run:
- Opens a fresh connection (leak).
- Fails to hit the already‑validated client, duplicating work.
- _, _, err := r.clientPool.Get(ctx, fmt.Sprintf("%d", o.ID))
+ _, _, err := r.clientPool.Get(ctx, o.Name)If the intent was to prove the DB accepts connections, the name is sufficient and consistent with other callers (DefaultDatabase, parseWithDatabaseID, etc.).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _, err := r.clientPool.Get(ctx, fmt.Sprintf("%d", o.ID)) | |
| if err != nil { | |
| var pgErr *pgconn.PgError | |
| if errors.As(err, &pgErr) { | |
| // Database does not accept connections | |
| if pgErr.Code == "55000" { | |
| l.Info("skipping database that does not accept connections", zap.String("database", o.Name), zap.Error(err)) | |
| continue | |
| } | |
| } | |
| return nil, "", nil, err | |
| } | |
| _, _, err := r.clientPool.Get(ctx, o.Name) | |
| if err != nil { | |
| var pgErr *pgconn.PgError | |
| if errors.As(err, &pgErr) { | |
| // Database does not accept connections | |
| if pgErr.Code == "55000" { | |
| l.Info("skipping database that does not accept connections", zap.String("database", o.Name), zap.Error(err)) | |
| continue | |
| } | |
| } | |
| return nil, "", nil, err | |
| } |
There was a problem hiding this comment.
Get method uses the databaseId, in string, not the name.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the implementation of the ClientDatabasesPool.Get method
ast-grep --pattern 'func (p *ClientDatabasesPool) Get(ctx context.Context, database string)' pkg/postgres/client.go
Length of output: 3046
@MarcusGoldschmidt You're absolutely right. Looking at the implementation of ClientDatabasesPool.Get, it definitely expects a database ID as a string:
func (p *ClientDatabasesPool) Get(ctx context.Context, database string) (*Client, string, error) {
// ...
dbModel, err := p.defaultClientDsn.GetDatabaseById(ctx, database)
// ...
}The method passes the string parameter directly to GetDatabaseById, confirming it expects an ID rather than a name. The current implementation using fmt.Sprintf("%d", o.ID) is correct.
Thanks for the correction!
🧬 Code Graph Analysis Results
Relevant snippets from pkg/postgres/databases.go (lines 17-22)
DatabaseModel struct {
ID int64 `db:"oid"`
Name string `db:"datname"`
OwnerID int64 `db:"datdba"`
ACLs []string `db:"datacl"`
}- Defines the
DatabaseModelstruct representing a database with fields includingID(int64) andName(string).
Relevant snippets from pkg/postgres/client.go (lines 13-22, 131-135)
ClientDatabasesPool struct {
databases map[string]*Client
opts []ClientOpt
mutex *sync.Mutex
logger *Logger
dsn string
// Only used if dsn have any database name
defaultClientWithDatabase *Client
defaultClientDsn *Client
}- Represents a pool of database clients keyed by string, with synchronization and configuration fields.
Client struct {
db *pgxpool.Pool
cfg *pgxpool.Config
schemaFilter []string
}- Represents a database client with connection pool and configuration.
Summary of database.go (entire file)
- The
databaseSyncerstruct manages database resources using apostgres.ClientDatabasesPooland a defaultpostgres.Client. - The
Getmethod is not explicitly defined here, but theGrantandRevokemethods use the database ID parsed from entitlement IDs (as string, then converted to int64) to look up the database viar.client.GetDatabase(ctx, dbID). - The
Listmethod lists databases and uses the database ID (converted to string) when callingr.clientPool.Get(ctx, fmt.Sprintf("%d", o.ID)). - The
CreateandDeletemethods use the database name for creation and deletion operations. - The resource IDs for databases are formatted using
formatObjectIDwith the database ID (int64) converted to string. - Privileges and grants are managed by parsing entitlement IDs that include the database ID as a string.
[no additional relevant snippets]
✏️ Learnings added
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-postgresql#31
File: pkg/connector/database.go:75-0
Timestamp: 2025-04-17T20:26:59.775Z
Learning: The `ClientDatabasesPool.Get` method in the PostgreSQL connector expects a database ID as a string parameter, not a database name. It passes this ID to `GetDatabaseById` to retrieve the database model.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| db, parentID, err := parseWithDatabaseID(parentResourceID.Resource) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| sequences, nextPageToken, err := r.client.ListSequences(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| client, _, err := r.clientPool.Get(ctx, db) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| sequences, nextPageToken, err := client.ListSequences(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| if err != nil { |
There was a problem hiding this comment.
Nil ‑ pointer risk with pToken
pToken may be nil; dereferencing it here causes a panic.
- sequences, nextPageToken, err := client.ListSequences(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size})
+ if pToken == nil {
+ pToken = &pagination.Token{}
+ }
+ sequences, nextPageToken, err := client.ListSequences(ctx, parentID, &postgres.Pager{
+ Token: pToken.Token,
+ Size: pToken.Size,
+ })Please propagate this pattern across the file.
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| client, _, err := r.clientPool.Get(ctx, database) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| schema, err := client.GetSchema(ctx, parentID) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| tables, nextPageToken, err := r.client.ListTables(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| tables, nextPageToken, err := client.ListTables(ctx, schema.Name, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| if err != nil { | ||
| return nil, "", nil, err |
There was a problem hiding this comment.
Nil ‑ pointer dereference risk (pToken)
Same pattern as other syncers – add a guard:
- tables, nextPageToken, err := client.ListTables(ctx, schema.Name, &postgres.Pager{Token: pToken.Token, Size: pToken.Size})
+ if pToken == nil {
+ pToken = &pagination.Token{}
+ }
+ tables, nextPageToken, err := client.ListTables(ctx, schema.Name, &postgres.Pager{
+ Token: pToken.Token,
+ Size: pToken.Size,
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| database, parentID, err := parseWithDatabaseID(parentResourceID.Resource) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| client, _, err := r.clientPool.Get(ctx, database) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| schema, err := client.GetSchema(ctx, parentID) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| tables, nextPageToken, err := r.client.ListTables(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | |
| tables, nextPageToken, err := client.ListTables(ctx, schema.Name, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | |
| if err != nil { | |
| database, parentID, err := parseWithDatabaseID(parentResourceID.Resource) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| client, _, err := r.clientPool.Get(ctx, database) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| schema, err := client.GetSchema(ctx, parentID) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| if pToken == nil { | |
| pToken = &pagination.Token{} | |
| } | |
| tables, nextPageToken, err := client.ListTables(ctx, schema.Name, &postgres.Pager{ | |
| Token: pToken.Token, | |
| Size: pToken.Size, | |
| }) | |
| if err != nil { |
pkg/connector/procedure.go
Outdated
| _, _, err := parseWithDatabaseID(resource.Id.Resource) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| // TODO: add db on entitlement | ||
| ens, err := entitlementsForPrivs(ctx, resource, postgres.Execute) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Entitlements lose the database context
The TODO highlights a real functional gap: entitlements generated for procedures from different databases will share the same ResourceId, leading to collisions and ambiguous grants when the connector syncs multiple DBs. Extend the entitlement construction helpers (and formatWithDatabaseID) so that the database component is embedded in the entitlement ID or slug, mirroring the resource ID strategy adopted elsewhere.
| client, _, err := r.clientPool.Get(ctx, db) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| procedures, nextPageToken, err := client.ListProcedures(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| if err != nil { | ||
| return nil, "", nil, err |
There was a problem hiding this comment.
Handle nil pagination token to avoid panics
pToken can be nil when the Baton SDK calls the first page of a collection.
Dereferencing it (pToken.Token, pToken.Size) will raise a panic, crashing the syncer.
- procedures, nextPageToken, err := client.ListProcedures(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size})
+ var pager *postgres.Pager
+ if pToken != nil {
+ pager = &postgres.Pager{Token: pToken.Token, Size: pToken.Size}
+ } else {
+ pager = &postgres.Pager{}
+ }
+
+ procedures, nextPageToken, err := client.ListProcedures(ctx, parentID, pager)This mirrors the defensive pattern used in several other syncers and eliminates a crash path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| client, _, err := r.clientPool.Get(ctx, db) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| procedures, nextPageToken, err := client.ListProcedures(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | |
| if err != nil { | |
| return nil, "", nil, err | |
| client, _, err := r.clientPool.Get(ctx, db) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| var pager *postgres.Pager | |
| if pToken != nil { | |
| pager = &postgres.Pager{Token: pToken.Token, Size: pToken.Size} | |
| } else { | |
| pager = &postgres.Pager{} | |
| } | |
| procedures, nextPageToken, err := client.ListProcedures(ctx, parentID, pager) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } |
| roles, nextPageToken, err := client.ListRoles(ctx, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } |
There was a problem hiding this comment.
Repeat of nil‑token issue in ListRoles call
The same pToken dereference occurs here. Please patch similarly:
- roles, nextPageToken, err := client.ListRoles(ctx, &postgres.Pager{Token: pToken.Token, Size: pToken.Size})
+ var pager *postgres.Pager
+ if pToken != nil {
+ pager = &postgres.Pager{Token: pToken.Token, Size: pToken.Size}
+ } else {
+ pager = &postgres.Pager{}
+ }
+ roles, nextPageToken, err := client.ListRoles(ctx, pager)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| roles, nextPageToken, err := client.ListRoles(ctx, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } | |
| var pager *postgres.Pager | |
| if pToken != nil { | |
| pager = &postgres.Pager{Token: pToken.Token, Size: pToken.Size} | |
| } else { | |
| pager = &postgres.Pager{} | |
| } | |
| roles, nextPageToken, err := client.ListRoles(ctx, pager) | |
| if err != nil { | |
| return nil, "", nil, err | |
| } |
MarcusGoldschmidt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/connector/sequence.go (1)
32-34:⚠️ Potential issueEarly return for nil checks is good, but inconsistent with line 50
While adding nil checks for both
parentResourceIDandpTokenis good defensive programming, there's a potential nil pointer dereference at line 50.if parentResourceID == nil || pToken == nil { return nil, "", nil, nil } // Later in the code... -sequences, nextPageToken, err := client.ListSequences(ctx, parentID, &postgres.Pager{Token: pToken.Token, Size: pToken.Size}) +sequences, nextPageToken, err := client.ListSequences(ctx, parentID, &postgres.Pager{ + Token: pToken.Token, + Size: pToken.Size, +})This doesn't fully address the nil-pointer risk mentioned in the previous review comment, as we're still directly accessing
pToken.TokenandpToken.Size.
🧹 Nitpick comments (3)
pkg/connector/sequence.go (3)
40-48: Properly implemented database ID parsing and client retrievalThe code correctly parses the database ID from the resource ID and retrieves the appropriate client from the pool.
Consider adding a comment about the format of the resource ID being parsed:
+// Parse the resource ID to extract the database ID and parent ID +// Format: {type}:{database}:{id} db, parentID, err := parseWithDatabaseID(parentResourceID.Resource) if err != nil { return nil, "", nil, err }
74-80: Database ID parsed but not used in Entitlements methodThe code parses the database ID and resource ID but doesn't use them in the
Entitlementsmethod.It appears the parsed values are intended for future implementation mentioned in the TODO comment. Consider either:
- Adding a comment explaining why the parsing is needed now but values aren't used, or
- Deferring the parsing until it's actually needed (when the TODO is implemented)
-_, _, err := parseWithDatabaseID(resource.Id.Resource) -if err != nil { - return nil, "", nil, err -} - -// TODO: add entitlements for sequences +// TODO: add entitlements for sequences (will need to parse database ID when implemented)
92-104: Correctly implemented database context in Grants methodThe Grants method properly extracts the database ID, retrieves the appropriate client, and uses it for subsequent operations.
Consider using a more descriptive variable name than
rIDto clarify that it's the sequence ID:-db, rID, err := parseWithDatabaseID(resource.Id.Resource) +db, sequenceID, err := parseWithDatabaseID(resource.Id.Resource) // Later in the code... -sequence, err := client.GetSequence(ctx, rID) +sequence, err := client.GetSequence(ctx, sequenceID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/connector/helpers.go(3 hunks)pkg/connector/procedure.go(4 hunks)pkg/connector/sequence.go(5 hunks)pkg/connector/table.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/connector/helpers.go
- pkg/connector/procedure.go
- pkg/connector/table.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/connector/sequence.go (1)
pkg/postgres/client.go (1)
ClientDatabasesPool(13-22)
🔇 Additional comments (3)
pkg/connector/sequence.go (3)
21-23: Correctly updated field to support multi-database connectivityThe change from a single client to a client pool aligns with the PR objective to support multiple databases.
63-63: Resource ID correctly formatted to include database contextUpdating the resource ID formatting to include the database ID ensures proper resource identification across multiple databases.
121-125: Constructor correctly updated to accept client poolThe constructor change properly aligns with the struct field change, ensuring consistent initialization.
pkg/connector/sequence.go
Outdated
| return nil, "", nil, err | ||
| } | ||
|
|
||
| // TODO: add entitlements for sequences |
There was a problem hiding this comment.
Right, but I need to add database name as prefix for this entitlement
pkg/connector/view.go
Outdated
| return nil, "", nil, err | ||
| } | ||
|
|
||
| // TODO: add dat |
There was a problem hiding this comment.
No, I need to add database name prefix for this entitlement, thanks
BB-450 Postgres Enhancements
Summary by CodeRabbit
New Features
Improvements
Bug Fixes